Add a session keyword argument to to_sql and to_df
#427
Conversation
… graph/config with the session in most places
knassre-bodo
requested review from
a team,
hadia206,
john-sanchez31 and
juankx-bodo
and removed request for
a team
| return SQLGlotRelationalVisitor(DatabaseDialect.SQLITE, config) | ||
| @pytest.fixture | ||
| def sqlglot_relational_visitor(empty_sqlite_tpch_session) -> SQLGlotRelationalVisitor: | ||
| return SQLGlotRelationalVisitor(empty_sqlite_tpch_session) |
There was a problem hiding this comment.
Can no longer be module-level since we depend on empty_sqlite_tpch_session
| raise PyDoughSessionException( | ||
| f"Expected `session` to be a PyDoughSession, got {session.__class__.__name__}." | ||
| ) | ||
| if session.metadata is None: |
There was a problem hiding this comment.
We are only checking for metadata. Should we check also for session.config and session.database?
Could we build new_session from session in kwargs if it exists, then if metadata, config or database are provided we overwrite them in new_session and finally if any of them is null in new_session then we set that value with its value in active_session.
There was a problem hiding this comment.
Same question, why only check for metadata. Why not config and database?
There was a problem hiding this comment.
Because session.metadata can be None according to the type (GraphMetadata | None), but session.config and session.database cannot.
| str, Callable[..., UnqualifiedNode], str, str | ||
| ], | ||
| verbose: bool, | ||
| get_sample_graph: graph_fetcher, |
There was a problem hiding this comment.
Should we provide the graph as part of the session?
There was a problem hiding this comment.
Because the way this test is written doesn't use sessions, it is just calling pydough.explain directly on metadata objects. The fetcher gets passed into this function (see the fixture metadata_exploration_test_data):
graph_name: str = args[0]
collection_name: str | None = args[1]
property_name: str | None = args[2]
def wrapped_test_impl(fetcher: graph_fetcher, verbose: bool):
graph: GraphMetadata = fetcher(graph_name)
if collection_name is None:
return pydough.explain(graph, verbose=verbose)
elif property_name is None:
return pydough.explain(graph[collection_name], verbose=verbose)
else:
return pydough.explain(
graph[collection_name][property_name], verbose=verbose
)
| ON _s1.o_custkey = customer.c_custkey | ||
| ORDER BY | ||
| total DESC | ||
| 3 DESC |
There was a problem hiding this comment.
Why did these queries change?
There was a problem hiding this comment.
While I was updating the documentation, I also updated the example generated SQL used in the documentation to reflect the most recent version of PyDough (since these are not automatically generated)
| raise PyDoughSessionException( | ||
| f"Expected `session` to be a PyDoughSession, got {session.__class__.__name__}." | ||
| ) | ||
| if session.metadata is None: |
There was a problem hiding this comment.
Same question, why only check for metadata. Why not config and database?
| "Please use `session.load_metadata_graph` to attach a graph to the session." | ||
| ) | ||
| if kwargs: | ||
| raise ValueError(f"Unexpected keyword arguments: {kwargs}") |
There was a problem hiding this comment.
Similar question to Juan's in the line up
What happens is the config and database are added? We should use them.
I understand that the metadata is required but others are optional but what happens if someone included those as well?
There was a problem hiding this comment.
They are all required. The difference is that the other two can't be None.
| str, Callable[..., UnqualifiedNode], str, str | ||
| ], | ||
| verbose: bool, | ||
| get_sample_graph: graph_fetcher, |
Resolves #421.